Fix build and tests for wolfSSL#352
Conversation
Signed-off-by: Juliusz Sosinowicz <juliusz@wolfssl.com>
prince-chrismc
left a comment
prince-chrismc
left a comment
There was a problem hiding this comment.
Thanks a ton for this! prince-chrismc#35 (comment) I've been staring at thos for a while so I really appreciate this.
I have a couple questions since I don't use wolfssl anymore I don't understand all the changes
| # This is required to access OpenSSL compatibility API | ||
| target_include_directories(jwt-cpp INTERFACE ${wolfssl_INCLUDE_DIRS}) | ||
| target_compile_definitions(jwt-cpp INTERFACE OPENSSL_EXTRA OPENSSL_ALL) | ||
| # EXTERNAL_OPTS_OPENVPN is necessary so that wolfssl/options.h is included automatically |
There was a problem hiding this comment.
Why was this header not required before? Would it be possible to just at the header?
There was a problem hiding this comment.
It was always necessary to include wolfssl/options.h. Notice that in your current builds you have
In file included from /usr/local/include/wolfssl/openssl/bn.h:33,
from /usr/local/include/wolfssl/openssl/ec.h:27,
from /home/runner/work/jwt-cpp/jwt-cpp/include/jwt-cpp/jwt.h:15,
from /home/runner/work/jwt-cpp/jwt-cpp/tests/JwksTest.cpp:1:
/usr/local/include/wolfssl/wolfcrypt/settings.h:2369:14: warning: #warning "For timing resistance / side-channel attack prevention consider using harden options" [-Wcpp]
2369 | #warning "For timing resistance / side-channel attack prevention consider using harden options"
| ^~~~~~~
which is a dead giveaway that wolfssl/options.h is missing. I think by defining OPENSSL_EXTRA OPENSSL_ALL you were able to somewhat build with wolfSSL but I don't know how the tests are "passing". I didn't investigate this.
|
Great to see someone from inside wolfSSL taking the time to improve support in opensource libraries. I noticed the symbol names for wolfSSL are different from openssl (hence the new |
Our compatibility layer headers are a set of macros that change the OpenSSL names to wolfSSL compatibility equivalents. Like |
This is failing in the OpenSSL without deprecated functions action. I think the solution would be for |
prince-chrismc
left a comment
prince-chrismc
left a comment
There was a problem hiding this comment.
This is so amazing helpful. Sorry for pushing the limits of the compatibility layer 🤣 but I see wolfSSL/wolfssl#7618 so hopefully it was not too bad.
The only change I am unsure about is the new defines added to CMake, I think that could be avoided, the openssl and libressl are not used --- it makes it inconsistent with how it was being referenced.
I have no issue with disabling the tests that are not support or have conflicts. The rest of the changes make sense but I did have a few questions just to confirm.
Thanks again for helping to improve this.
this is deprecated with 3.0
|
I appreciate the review. I will try to get back to this PR next week. |
prince-chrismc
left a comment
prince-chrismc
left a comment
There was a problem hiding this comment.
playing with getting the right headers pulled in
|
🤞 this should be ready. Thanks so much for contributing to both side to make the integration better 🚀 I wont be updating the docs with the new tested version because I have a PR to update the the SSL versions which would conflict. |
|
Thanks for helping out here. I haven't had time to bring it across the finish line in the past month. |
3 participants
Tested with wolfSSL master